[SSF-159] - Automated Emails 1 Implementation#127
Conversation
Yurika-Kan
left a comment
Yurika-Kan
left a comment
There was a problem hiding this comment.
super great pr! super cool to test this & see emails come in as if i am the end user of this site :)
regarding the order of either saving to the db then sending emails and vice versa (specifically in request & user), i'd suggest to save the data first then send the appropriate email in cases that email sending fails. ideally we wouldn't wait for the email sending to succeed before saving that data. db is the source of truth & emails are more so secondary/side effect!
tested via:
- verified email -> postman -> receiving emails
- jest tests pass
thanks for adding all these tests btw!
There was a problem hiding this comment.
file name typo: manufaacturers --> manufacturers
| bodyHTML: string, | ||
| attachments?: EmailAttachment[], | ||
| ): Promise<unknown> { | ||
| if (process.env.SEND_AUTOMATED_EMAILS === 'false') { |
There was a problem hiding this comment.
if this env variable is not set, then it would default to sending the emails~ could we treat email sending as enabled only when it is true & everything else (false or nothing/default), ie when it's not true, is disabled?
|
|
||
| const message = emailTemplates.pantrySubmitsFoodRequest({ | ||
| pantryName: pantry.pantryName, | ||
| volunteerName: pantry.pantryUser.firstName, |
There was a problem hiding this comment.
we need to take in the volunteer name here!
| await service.deny(id); | ||
|
|
||
| const denied = await service.findOne(id); | ||
| expect(denied.status).toBe(ApplicationStatus.DENIED); |
There was a problem hiding this comment.
can we check here that it does not send any email when denying
There was a problem hiding this comment.
could we add tests for 2 edge cases:
- email fails before request is saved --> current functionality is that request will not be saved
- zero volunteers associated with a pantry --> create succeeds, sendEmails succeeds with empty recipient array, request is saved
|
|
||
| await service.approve(5); | ||
|
|
||
| expect(mockEmailsService.sendEmails).toHaveBeenCalledWith( |
There was a problem hiding this comment.
can we also check it as called once
| userCognitoSub, | ||
| }); | ||
| return this.repo.save(user); | ||
| await this.repo.save(user); |
There was a problem hiding this comment.
note: in requests we send email first then save where as here, we save then send email
i think this design makes more sense
| // Send welcome email to new volunteers | ||
| if (role === Role.VOLUNTEER) { | ||
| const message = emailTemplates.volunteerAccountCreated(); | ||
| await this.emailsService.sendEmails( |
There was a problem hiding this comment.
although the await can fail the flow / hang
2 participants
ℹ️ Issue
Closes #159
📝 Description
✔️ Verification
BEFORE TESTING: Add 2 new environment variables:
Add your email that you put in the AWS_SES_SENDER_EMAIL into the following AWS SES Identities: https://us-east-2.console.aws.amazon.com/ses/home?region=us-east-2#/identities
Tested each workflow to ensure the proper sender, subject, message, attachments were all there.
🏕️ (Optional) Future Work / Notes
Should figure out why this email gets sent to spam by default